Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(ci): automated issue labeling #41

Merged
merged 4 commits into from
Jan 9, 2022
Merged

chore(ci): automated issue labeling #41

merged 4 commits into from
Jan 9, 2022

Conversation

Shinigami92
Copy link
Member

No description provided.

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jan 9, 2022
@Shinigami92 Shinigami92 self-assigned this Jan 9, 2022
griest024
griest024 suggested changes Jan 9, 2022
View reviewed changes
Copy link
Contributor

@griest024 griest024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think each step deserves a brief comment saying the intention of the step.

.github/workflows/issue-labeling.yml Show resolved Hide resolved
@Shinigami92
Copy link
Member Author

I think each step deserves a brief comment saying the intention of the step.

Are the name of the steps not already self-explanatory?

@griest024
Copy link
Contributor

I think each step deserves a brief comment saying the intention of the step.

Are the name of the steps not already self-explanatory?

I don't think the conditionals are self-explanatory.

@Shinigami92
Copy link
Member Author

I don't think the conditionals are self-explanatory.

I mean

https://github.com/faker-js/faker.js/blob/46bc978163c1e1aab1c26fe6ec10822c9a214a09/.github/workflows/issue-labeling.yml#L11

and

https://github.com/faker-js/faker.js/blob/46bc978163c1e1aab1c26fe6ec10822c9a214a09/.github/workflows/issue-labeling.yml#L22

@griest024
Copy link
Contributor

I don't think the conditionals are self-explanatory.

I mean

https://github.com/faker-js/faker.js/blob/46bc978163c1e1aab1c26fe6ec10822c9a214a09/.github/workflows/issue-labeling.yml#L11

and

https://github.com/faker-js/faker.js/blob/46bc978163c1e1aab1c26fe6ec10822c9a214a09/.github/workflows/issue-labeling.yml#L22

What I mean is, I don't think the names of the steps explain why the conditionals are what they are. From a high level, what is the step trying to do? Why even check the label names at all? These are the questions that I would want answered by a comment.

damienwebdev
damienwebdev approved these changes Jan 9, 2022
View reviewed changes
Copy link
Member

@damienwebdev damienwebdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Shinigami92
Copy link
Member Author

@griest024 Can I receive your approval anyway? I added the comments 🙂
And still: if something is wrong, we can change it

@damienwebdev damienwebdev merged commit 0c8506f into main Jan 9, 2022
@damienwebdev damienwebdev deleted the Shinigami92/add-automated-issue-labeling branch January 9, 2022 20:45
@Shinigami92
Copy link
Member Author

Test was done in #45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@griest024 griest024 griest024 approved these changes

@damienwebdev damienwebdev damienwebdev approved these changes

Assignees

@Shinigami92 Shinigami92

Labels
c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Shinigami92 @griest024 @damienwebdev